-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove CodeQL, add flake8 #153
Conversation
Let's use something with in-code annotations instead.
"%s %s".format(1, 2) # This should have been '{} {}' '%s %s'
0696254
to
c28b6ac
Compare
Should not compare with !=.
c28b6ac
to
a69e9ec
Compare
Now that things are fixed up, add flake8. No fancy plugins, just base flake8 for now.
a69e9ec
to
77bcde4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From skimming over the changes, this looks good to me.
IMO it would be a good idea to add additional linters to cover more scenarios here or as a follow-up, e.g., pylint and bandit.
Yeah, not sure about about having flake8 and pylint due to possibly needing to appease them with different annotations, bandit could be interesting. There's also a bunch of flake8 plugins if we wanted to go wild: https://github.com/DmytroLitvinov/awesome-flake8-extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Arne, one less thing for me to do today.
@ckreibich , this builds on top of your #152 but at the same time adds a flake8 pre-commit hook.
Nothing critical stands out. A bit of logging broke during #151 , otherwise minor things around unused variables, import orderings and comparisons with bool.
Prefer to add some form of linter rather than having none at all.